-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new setting to disable persistent tasks allocations #29137
Conversation
Pinging @elastic/es-distributed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you open an issue for the follow-up that you proposed at the end of your PR so it wouldn't get lost? Without such follow-up this PR doesn't make a lot of sense. Ideally, I would have preferred for this follow-up to happen first and this change to follow, but it might be not possible if we need this for 6.3.
*/ | ||
public final class AssignmentDecision { | ||
|
||
public static final AssignmentDecision ALWAYS = new AssignmentDecision(Type.YES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels like premature optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 . People can just use yes? I'm not sure what the difference is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's not required.
Yes, the follow up is a bigger change and we'd like the setting to be added in 6.3, thus the order of pull requests. Thanks for your review Igor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx @tlrx . I left some suggestions, mostly about removing stuff that are not yet needed.
I would love to have an integration test (i.e., qa) to check that what we need (setting this in the es.yml file actually prevents initial allocation of tasks and that you can enable them from the API). This may be tricky so we need to check whether we can do this in time.
Re your suggestion of opening up the deciders to the executors - I'll reach you to talk in another channel. I want to understand better where you see this heading
@@ -45,12 +48,14 @@ | |||
|
|||
private final ClusterService clusterService; | |||
private final PersistentTasksExecutorRegistry registry; | |||
private final AssignmentDecider<PersistentTaskParams> decider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just hard code this for now to EnableAssignmentDecider? we can generalize when it becomes relevant (and know what we want exactly)
* {@link AssignmentDecider} are used to decide if a persistent | ||
* task can be assigned to a node. | ||
*/ | ||
public abstract class AssignmentDecider<Params extends PersistentTaskParams> extends AbstractComponent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this to be added at the same time we support multiple deciders.
*/ | ||
public final class AssignmentDecision { | ||
|
||
public static final AssignmentDecision ALWAYS = new AssignmentDecision(Type.YES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 . People can just use yes? I'm not sure what the difference is?
|
||
public static final AssignmentDecision ALWAYS = new AssignmentDecision(Type.YES); | ||
public static final AssignmentDecision YES = new AssignmentDecision(Type.YES); | ||
public static final AssignmentDecision NO = new AssignmentDecision(Type.NO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want this? it means people can say no without giving a reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right
* | ||
* @see Allocation | ||
*/ | ||
public class EnableAssignmentDecider<Params extends PersistentTaskParams> extends AssignmentDecider<Params> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this generic?
This PR removes the method getAssignment() for the PersistentTasksExecutor in favor of `getTaskAssignmentDecider()` that allows the executor to declare the AssignmentDecider to use when assigning the tasks. This decider is combined with the built-in ones in order to allow the PersistentTasksClusterService to decide if a task can be assigned or not, and on which nodes. Then, the final choice of the assigned node is let to the executor that can apply its custom logic. The goal is to allow top level decisions for persistent tasks assignment in the PersistentTasksClusterService (see elastic#29137) while splitting the logic of task assignment in smaller chunks into dedicated deciders, because persistent tasks use some similar rules (like "assign to data node only", "node must be in version X" etc) that can be shared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @tlrx
This commit adds a new setting `cluster.persistent_tasks.allocation.enable` that can be used to enable or disable the allocation of persistent tasks. The setting accepts the values `all` (default) or `none`. When set to none, the persistent tasks that are created (or that must be reassigned) won't be assigned to a node but will reside in the cluster state with a no "executor node" and a reason describing why it is not assigned: ``` "assignment" : { "executor_node" : null, "explanation" : "persistent task [foo/bar] cannot be assigned [no persistent task assignments are allowed due to cluster settings]" } ``` This commit adds some AssignmentDecider and AssignmentDecision classes that are similar to the deciders/decisions classes used for shard allocations. It also adds an EnableAssignmentDecider decider that is responsible for registering the new cluster settings. The setting is checked when executing the PersistentTasksClusterService `createAssignment()` method, before delegating the creation of the assignment to the PersistentTasksExecutor. As a follow up, I think that PersistentTasksExecutor could expose a `getTaskAllocationDecider` method that would be used by the PersistentTasksClusterService to build a list of nodes that could execute the new task. Then this list of assignable nodes could be passed again to the task executor to select the exact node to execute the task.
77db006
to
3648a2e
Compare
This commit adds a new setting `cluster.persistent_tasks.allocation.enable` that can be used to enable or disable the allocation of persistent tasks. The setting accepts the values `all` (default) or `none`. When set to none, the persistent tasks that are created (or that must be reassigned) won't be assigned to a node but will reside in the cluster state with a no "executor node" and a reason describing why it is not assigned: ``` "assignment" : { "executor_node" : null, "explanation" : "persistent task [foo/bar] cannot be assigned [no persistent task assignments are allowed due to cluster settings]" } ```
Thanks @bleskes |
This commit adds a new setting
cluster.persistent_tasks.allocation.enable
that can be used to enable or disable the allocation of persistent tasks.The setting accepts the values
all
(default) ornone
. When set to none, the persistent tasks that are created (or that must be reassigned) won't be assigned to a node but will reside in the cluster state witha
null
executor node and a reason describing why it is not assigned:This commit adds some AssignmentDecider and AssignmentDecision classes that are similar to the deciders/decisions classes used for shard allocations. It also adds an EnableAssignmentDecider decider that is responsible for registering the new cluster setting.
The setting is checked when executing the
PersistentTasksClusterService#createAssignment()
method, before delegating the creation of the assignment to the PersistentTasksExecutor. As a follow up, I think that PersistentTasksExecutor could expose agetTaskAllocationDecider
method that would be used by the persistent task cluster service to build a list of nodes that could execute the new task. Then this list of assignable nodes could be passed again to the executor to select the exact node to execute the task.